Skip to content

close #75 {DO NOT MERGE} refactor collision detection logic from player class.#78

Open
pwentz wants to merge 1 commit intomasterfrom
75-refactor-collision-detection
Open

close #75 {DO NOT MERGE} refactor collision detection logic from player class.#78
pwentz wants to merge 1 commit intomasterfrom
75-refactor-collision-detection

Conversation

@pwentz
Copy link
Collaborator

@pwentz pwentz commented Oct 18, 2016

What's this PR do?

  • Move logic for determining incoming collisions into collision engine class.
  • Gave collision engine an object instance instead of binding one on method calls
  • CollisionEngine now calculates all possible colliders with its object, and returns a boolean on whether the player/fire-block/bomb can occupy the corresponding space.
  • Topic of refactor focused around Law of Demeter. However by moving the logic off of the player class and into the CollisionEngine class - it appears that CollisionEngine is doing a lot of logic for a lot of different scenarios. Also attempted to break functions out into objects similar to the way the lesson had suggested (colliisionEngine - lines 6-24) - but unsure of what practical value it provides.

Where should the reviewer start?

  • Scroll ALL the way to the bottom and check out the code that had initially been on the player class and compare to the new function in its place.
  • Then checkout the other places in which the CollisionEngine methods are being called.
  • Checkout the CollisionEngine class (specifically lines 6-24)

@pwentz pwentz changed the title close #75 refactor collision detection logic from player class. close #75 {DO NOT MERGE} refactor collision detection logic from player class. Oct 18, 2016
Copy link
Collaborator Author

@pwentz pwentz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant